Skip to content

Fix Share-for-review so the owner gets a working invite URL#5

Merged
angusbezzina merged 5 commits into
mainfrom
fix/share-dialog-shows-invite-url
Jun 10, 2026
Merged

Fix Share-for-review so the owner gets a working invite URL#5
angusbezzina merged 5 commits into
mainfrom
fix/share-dialog-shows-invite-url

Conversation

@angusbezzina

@angusbezzina angusbezzina commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Problem

Clicking Share for review as the owner produced a misleading "Couldn't reach the review relay — the share didn't complete." error after a ~15s spin, and never showed the invite URL — even though the backend share actually succeeded (the room was created and the snapshot uploaded). With no URL to copy, the owner couldn't invite anyone.

Root cause

The invite URL was gated behind shareTargetIsCurrent, which scanned reviewStore.snapshots for the shared path. That store is populated for reviewers (they receive a snapshot_created event) but never for the owner's own fresh share — the daemon drops the owner's snapshot envelope and ReviewUpdate::ShareReady carried no path. So the gate was always false for the owner → the URL was blanked → the 15s mint timeout fired with an empty message, rendering the generic relay error.

Fix

  • Thread the shared path to the frontend. ReviewUpdate::ShareReady now carries owner_display_path (the verbatim path the owner shared), populated at both share() outcome sites and forwarded by emit_share_outcome. applyShareReady stamps it onto currentShare, and the dialog's gate matches against it via a pure, unit-tested shareTargetMatches(). Works for both GUI shares and attn review share <path> from the CLI — no fragile frontend-captured intent.
  • Recover from a slow relay. If ShareReady lands after the 15s timeout already flipped the dialog to "error", it now recovers and shows the invite rather than leaving a spurious error (a genuine failure never populates the URL, so this can't mask a real error).

Local collab harness (task dev:collab)

Test-tooling fixes that make the owner↔reviewer flow exercisable locally:

  • Forward ATTN_RELAY_URL to the booted daemons (they were falling back to the no-op stub, so Share never reached the relay).
  • Open the reviewer on a different fixture (typography.md) so the two otherwise-identical windows are tellable apart — share from the "Project Status" window; the reviewer window visibly switches to it on join. Warns if the two fixtures collide.

Robustness (from an xhigh self-review of the diff)

Scoped applyError (no longer discards an in-flight intent on an unrelated room's error), removed a redundant/over-eager effect that double-cleared and re-fired, resolved the reviewer-fixture default at call time, and reset the dialog's share target on close. The owner_display_path threading above superseded the original frontend-intent approach and dissolved the cross-stamp / CLI-path edge cases the review surfaced.

Testing

  • Rust: cargo test review suite 413/0, incl. a new assertion that ShareReady round-trips the exact shared path.
  • Web: tests 31/0 (incl. shareTargetMatches cases), svelte-check 0 errors, vite build OK.
  • Binary-size gate: 30.20 MiB / 32 MiB.
  • E2E: validated the full owner→reviewer round-trip locally (Miniflare relay) — reviewer joins, switches to the shared doc, both show a Live peer — and the owner share happy-path against the production relay.

After merge + release

Release builds bake in the production relay (ATTN_DEFAULT_RELAY_URL=https://relay.attn.sh, already in release.yml), so a released binary shares out of the box: owner clicks Share → gets npx attnmd review join 'attn://…' → sends it → reviewer joins → live collaboration.

Deferred (not blocking)

  • The owner's file-tree "shared" badge and owner-side room name still derive from reviewStore.snapshots, which remains empty for the owner (the daemon doesn't echo the owner's own snapshot to the frontend). Cosmetic; pre-existing.
  • shareTargetMatches does raw-string path comparison (no symlink/../case canonicalization) — low risk now that the path is single-sourced from the daemon.

🤖 Generated with Claude Code

Clicking Share on a markdown file succeeded on the backend (relay
round-trip + ShareReady with a real invite URL) but the dialog timed out
after 15s with "Couldn't reach the review relay" and never showed the URL.

Root cause: commit e1bf35d gated the invite URL on `shareTargetIsCurrent`,
which scanned `reviewStore.snapshots` for the shared path. That store is
populated for reviewers (who receive a snapshot_created event) but never
for the owner's own fresh share -- the daemon drops snapshot envelopes in
forward_transport_event and ShareReady carries no path. So the gate was
always false for the owner, blanking the URL until the 15s mint timeout.

Fix (frontend-only, narrow blast radius): thread the path the frontend
already knows onto the share record. ShareDialog.onStart records the
in-flight path via reviewStore.noteShareIntent() when reviewShare fires;
applyShareReady stamps it as currentShare.ownerDisplayPath (persisted on
the room.share record so reselect/rehydration keeps it); shareTargetIsCurrent
now matches against that path via a new pure shareTargetMatches() in
room-ui.ts. A failed/timed-out mint releases the intent (clearShareIntent /
onAbort + applyError) so a stale path can't bleed into a later share.

Preserves single-file, folder, child-of-folder, and unshared-file
semantics. No daemon change.

Tests: 6 new shareTargetMatches cases in room-ui.test.ts (tsx defineCase
harness; the project has no vitest).

Deferred follow-up: emit the owner's own snapshot to the frontend at share
time so reviewStore.snapshots is populated for the owner -- the
architecturally complete daemon fix that would also repair the file-tree
"shared" marker and owner roomDisplayName (both already broken for the
owner before this gate, out of scope here).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jun 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
attn Ready Ready Preview, Comment Jun 9, 2026 10:51pm

Request Review

`scripts/dev-collab.sh` set ATTN_RELAY_URL but never exported it, and
`start_dual` (scripts/lib/dual-instance.sh) launched the owner/reviewer
daemons with only ATTN_HOME prefixed. So the daemons started with no relay
configured and fell back to the scaffold stub -- Share never reached the
relay and the dialog timed out, making the collab flow impossible to test
locally even though the relay was healthy.

Forward ATTN_RELAY_URL (defaulting to empty, which the daemon treats as
"unset") on both daemon launches in start_dual, and export it from
dev-collab.sh. Empty stays safe for callers that don't want a relay; when
set, the daemons now attach the real relay.

Verified end to end: `task dev:collab` owner now logs `review bootstrap
attached (relay=http://localhost:8787)`, Share publishes a snapshot and the
dialog shows the invite, and the reviewer joins over ws://localhost:8787.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@angusbezzina

Copy link
Copy Markdown
Collaborator Author

Validated end-to-end locally ✅

Added a second commit fixing the local collab harness: scripts/dev-collab.sh set ATTN_RELAY_URL but never exported it, and start_dual launched the owner/reviewer daemons with only ATTN_HOME — so the daemons fell back to the scaffold stub and Share never reached the relay, making this fix impossible to exercise via task dev:collab.

E2E evidence (driven via the debug-build automation flags):

  • Against the production relay: owner Share → dialog reached the ready state showing npx attnmd review join 'attn://review/…#key=…'; daemon logged published snapshot … room=…started room runtime … outbox+ws subscribed. Pre-fix this exact sequence timed out at 15s.
  • Against the local Miniflare relay (task dev:collab with the harness fix): owner now logs review bootstrap attached (relay=http://localhost:8787), Share shows the invite, and the reviewer joins over ws://localhost:8787 — its window switches to the shared doc with a "Live" connection badge and review bar.

So both layers are confirmed: the dialog surfaces the invite URL for the owner, and the full owner→reviewer round-trip works locally.

…ellable apart

The owner and reviewer windows both opened the same fixture and were
pixel-identical, so it was easy to click Share on the wrong one. Since the
join always routes to the reviewer daemon, sharing from the reviewer window
loops the invite back to itself (joins its own room, "No peers") while the
owner window sits idle — looking like collab silently failed.

Open the reviewer on a different fixture (default typography.md) via a new
ATTN_DUAL_REVIEWER_FIXTURE knob (defaults to the owner's fixture for
backward compat). Now the owner window shows basic.md / "Project Status" and
the reviewer shows typography.md until it joins, at which point it switches
to the owner's doc — making both "which window do I share from" and "did the
join work" visually obvious. Terminal hints updated to say SHARE FROM the
owner window.

Verified: owner h1 "Project Status" vs reviewer h1 "Heading Level 1"; after
owner Share + reviewer join, reviewer h1 -> "Project Status" and both review
bars show the other peer (owner "…Live R", reviewer "Live O").

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From the xhigh review of this branch:

#1 (ShareReady after the 15s timeout): a slow relay could answer after the
mint timeout already flipped the dialog to 'error' and the onAbort effect had
cleared the share-intent, so a *successful* share showed a spurious error and
"Try again" minted a duplicate room. Fixed two ways: the ready transition now
also recovers from the 'error' state when a valid inviteUrl arrives, and the
over-eager onAbort clear is removed (a genuine daemon failure never populates
inviteUrl, so recovery can't mask a real error).

#5/#8 (onAbort effect): the `$effect(() => { if (phase==='error') onAbort?.() })`
lacked the `if (!open) return` guard its siblings have and re-fired on every
parent re-render (fresh closure dep); it also double-cleared the intent
alongside applyError. Removed the effect, the onAbort prop, and the now-dead
clearShareIntent() store method.

#3 (applyError scope): applyError cleared pendingShareTargetPath for ANY review
error, so an unrelated error on another live owner room during the ~15s mint
window discarded a valid in-flight share intent. Now it clears only on a
non-room-scoped error (room_id: None — what a failed share emits).

#6 (dual-instance reviewer fixture): ATTN_DUAL_REVIEWER_FIXTURE was defaulted
at source time, so a caller that set ATTN_DUAL_FIXTURE after sourcing got a
reviewer fixture pinned to the old default. Resolved at start_dual call time
via a local instead.

Verified: web tests 31/0, svelte-check 0 errors, vite build OK, bash -n OK,
and a share happy-path smoke against the relay still reaches the invite URL.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent seam

The durable fix for review finding #9 (and the #2/#4/#7 family that stemmed
from it). The owner's shared path is now carried on the ShareReady payload
straight from the daemon, instead of being reconstructed on the frontend via a
captured-intent slot.

Rust: ShareOutcome and ReviewUpdate::ShareReady gain owner_display_path
(= path.to_string_lossy(), the verbatim path the frontend passed to
reviewShare). Populated at both share() outcome sites (fresh mint + re-share)
and forwarded by emit_share_outcome. New assertion in
share_emits_invite_with_expected_shape locks the round-trip.

Frontend: applyShareReady stamps currentShare.ownerDisplayPath from
payload.ownerDisplayPath. Removed pendingShareTargetPath, noteShareIntent, the
ShareDialog onStart wiring, and applyError's intent-clear — the whole seam is
gone. This dissolves the earlier findings it caused:
  - #2 un-keyed cross-stamp (no global slot anymore),
  - #4 / #7 CLI `attn review share` yielding ownerDisplayPath='' (the path now
    comes from the daemon for CLI and GUI shares alike),
  - the noteShareIntent-timing footgun.

Also resolves two smaller findings:
  - #11: App resets shareTargetPath when the share dialog closes so a stale
    path can't feed shareTargetIsCurrent on the next open.
  - sweep: dev:collab warns when the owner and reviewer fixtures collide
    (windows would be indistinguishable).

This change is genuinely testable where the seam was not: the path logic now
lives in Rust (cargo test), not the un-unit-testable runes store.

Verified: cargo review tests 413/0 (incl. the new path assertion), web tests
31/0, svelte-check 0 errors, vite build OK, bash -n OK, and a share happy-path
smoke against the relay shows the invite (gate matches the daemon-supplied
path).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@angusbezzina angusbezzina marked this pull request as ready for review June 10, 2026 01:27
@angusbezzina angusbezzina changed the title Fix owner Share dialog timing out instead of showing the invite URL Fix Share-for-review so the owner gets a working invite URL Jun 10, 2026
@angusbezzina angusbezzina merged commit f48143b into main Jun 10, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant